Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reinstate DynamicSmagorinsky tests #3926

Merged
merged 10 commits into from
Dec 13, 2024
Merged

Reinstate DynamicSmagorinsky tests #3926

merged 10 commits into from
Dec 13, 2024

Conversation

glwagner
Copy link
Member

Opening this PR so we can continue tinkering with the DynamicSmagorinsky tests to see if we can get them to run.

cc @tomchor

@tomchor
Copy link
Collaborator

tomchor commented Nov 14, 2024

@glwagner thanks for opening this. Let me know how you'd like me to help here.

@tomchor
Copy link
Collaborator

tomchor commented Nov 15, 2024

Just FYI, dynamic Smagorinskys compile and run successfully over at Oceanostics: tomchor/Oceanostics.jl#183

@glwagner
Copy link
Member Author

Then perhaps the cause is the flag -O0 that we use for init.

You could try adding a github actions build here. Might be nice to have that anyways

@tomchor
Copy link
Collaborator

tomchor commented Nov 15, 2024

Then perhaps the cause is the flag -O0 that we use for init.

I tried that locally and it didn't work (at least on my laptop).

You could try adding a github actions build here. Might be nice to have that anyways

Just for the turbulence tests? What are you envisioning?

@glwagner
Copy link
Member Author

Then perhaps the cause is the flag -O0 that we use for init.

I tried that locally and it didn't work (at least on my laptop).

You could try adding a github actions build here. Might be nice to have that anyways

Just for the turbulence tests? What are you envisioning?

Is there a need to add more tests to it? I mean in the context of this PR, to get DynamicSmagorinsky tests to pass.

@tomchor
Copy link
Collaborator

tomchor commented Nov 15, 2024

You could try adding a github actions build here. Might be nice to have that anyways

Just for the turbulence tests? What are you envisioning?

Is there a need to add more tests to it? I mean in the context of this PR, to get DynamicSmagorinsky tests to pass.

I don't think we need to add more tests for dynamic Smagorinsky if that's your question. The tests we have should be fine, we just need to make them pass.

@glwagner
Copy link
Member Author

You could try adding a github actions build here. Might be nice to have that anyways

Just for the turbulence tests? What are you envisioning?

Is there a need to add more tests to it? I mean in the context of this PR, to get DynamicSmagorinsky tests to pass.

I don't think we need to add more tests for dynamic Smagorinsky if that's your question. The tests we have should be fine, we just need to make them pass.

No, I'm not saying we should add more tests. I'm suggesting we move the turbulence closure tests to github actions in order to get them to pass.

@tomchor
Copy link
Collaborator

tomchor commented Nov 15, 2024

You could try adding a github actions build here. Might be nice to have that anyways

Just for the turbulence tests? What are you envisioning?

Is there a need to add more tests to it? I mean in the context of this PR, to get DynamicSmagorinsky tests to pass.

I don't think we need to add more tests for dynamic Smagorinsky if that's your question. The tests we have should be fine, we just need to make them pass.

No, I'm not saying we should add more tests. I'm suggesting we move the turbulence closure tests to github actions in order to get them to pass.

Yeah, if there isn't much onus to with having another test suite to deal with, I'm okay with that. I think we can only test CPU architecture there though.

@glwagner
Copy link
Member Author

@tomchor
Copy link
Collaborator

tomchor commented Nov 15, 2024

It seems you need an enterprise account to use them:

image

Does Clima have such an account?

@glwagner
Copy link
Member Author

It seems you need an enterprise account to use them:

image

Does Clima have such an account?

I don't think so. So yeah, we would be limited to CPU tests. It could be better than nothing?

@tomchor
Copy link
Collaborator

tomchor commented Dec 11, 2024

@glwagner I think I got this working properly. I just need to add back the rest of the tests. As it is at the moment, the full CPU turbulence closures test is being run on Github Actions and the dynamic Smagorinsky seems to be working well there, as expected. All other tests, including the GPU turbulence tests, are run via buildkite as usual. Surprisingly, the GPU turbulence tests seem to pass now, including the dynamic Smag stuff.

I am getting an error about there not being enough space on device for GPU tests at the moment, so I'm guessing something needs to be deleted there:

image

@glwagner
Copy link
Member Author

Nice! I'll fix the GPU situation. Curious to get @simone-silvestri 's feed back about adding a github actions pipeline too.

@simone-silvestri
Copy link
Collaborator

It's very nice to split up the tests in more pipelines to reduce testing costs.
It's a pity we cannot use the GPUs as the GPU pipeline is the most expensive, but maybe that option presents itself in the future. Starting with CPU is a good idea for me.

@tomchor
Copy link
Collaborator

tomchor commented Dec 12, 2024

It's very nice to split up the tests in more pipelines to reduce testing costs. It's a pity we cannot use the GPUs as the GPU pipeline is the most expensive, but maybe that option presents itself in the future. Starting with CPU is a good idea for me.

It can be done, but that's a paid github feature. So there would need to be some continuous funding for that I guess.

@glwagner Can you let me know when I can restart the GPU tests? It seems to still be giving me the out of storage error.

@glwagner glwagner merged commit a8b3306 into main Dec 13, 2024
46 checks passed
@glwagner glwagner deleted the glw/dynamic-smag-tests branch December 13, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants